Fix #6497: Support global flags passed in after subcommands #8670

aranke · 2023-09-19T13:05:38Z

resolves #6497

Problem

Solution

Checklist

I have read the contributing guide and understand what's expected of me
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

codecov · 2023-09-19T13:07:56Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (aa86fdf) 86.61% compared to head (168f846) 82.74%.

❗ Current head 168f846 differs from pull request most recent head 913b1cd. Consider uploading reports for the commit 913b1cd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8670      +/-   ##
==========================================
- Coverage   86.61%   82.74%   -3.87%     
==========================================
  Files         176      176              
  Lines       25678    25692      +14     
==========================================
- Hits        22240    21260     -980     
- Misses       3438     4432     +994

Flag	Coverage Δ
integration	`77.81% <100.00%> (-5.56%)`	⬇️
unit	`65.17% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
core/dbt/cli/main.py	`96.64% <100.00%> (-2.06%)`	⬇️

... and 62 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jtcohen6

Mea culpa - I had missed adding the 1.5 + 1.6 backport labels to #6497. Ideally we could offer the same consistency for all our CLIs starting with v1.5+. Given how "clean" this change looks, how would you feel about backporting the change (at least the approach if not the exact commit diff) to 1.5.latest + 1.6.latest?

jtcohen6 · 2023-09-20T10:02:34Z

core/dbt/cli/main.py

@@ -118,6 +119,44 @@ def invoke(self, args: List[str], **kwargs) -> dbtRunnerResult:
            )


+# approach from https://github.com/pallets/click/issues/108#issuecomment-280489786
+def global_flags(func):


This is neat!

aranke · 2023-09-20T10:24:47Z

@jtcohen6 These flags need to be set at the global level, and there doesn't seem to be an easy way to globalize them everywhere:

@p.warn_error
@p.warn_error_options
@p.log_format

Do you want me to continue working on this, or should I merge this PR as-is and create a backlog ticket to handle these edge cases?

aranke · 2023-09-20T10:28:15Z

Given how "clean" this change looks, how would you feel about backporting the change (at least the approach if not the exact commit diff) to 1.5.latest + 1.6.latest?

I'm fine with back porting the PR in its current state to 1.5 and 1.6, but if the acceptance criteria include back porting the three errant flags highlighted above, I might need to rethink.

jtcohen6 · 2023-09-20T11:03:34Z

@aranke That's odd - it's only those three flags which are being difficult? Do you have any sense of why?

aranke · 2023-09-20T14:10:51Z

Do you have any sense of why?

I'm not certain, honestly, but could be that they are being passed down incorrectly.
I can keep digging for a little, but don't want to delay this PR if there isn't an easy fix.

aranke · 2023-09-26T07:35:51Z

I've tried many combinations, and it looks like the order in which the parameter decorators are passed in matters.

Creating a backlog ticket (#8715) to investigate further, but I don't think we need to hold up this PR to globalize the three remaining flags.

jtcohen6 · 2023-09-26T10:54:49Z

@aranke Thanks for investigating & opening the ticket! We should try to get that tech debt ticket resolved — it's important for our ability to present consistent documentation & CLI behavior (dbt-labs/docs.getdbt.com#4041) — but I agree that it shouldn't block merging the brunt of the work in this PR.

emmyoop

LGTM

FishtownBuildBot · 2023-09-26T15:17:12Z

Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#4129

ChenyuLInx

Thanks!

ChenyuLInx · 2023-09-26T15:18:26Z

core/dbt/cli/main.py

@@ -167,10 +178,10 @@ def cli(ctx, **kwargs):
 # dbt build
 @cli.command("build")
 @click.pass_context
+@global_flags


Nice change! What happens when a flag in global flags got passed in twice at different level with different value, which one wins?

We throw an error, similar to existing behavior.

Product guidance from jerco: #6497 (comment)

Test:

dbt-core/tests/unit/test_cli_flags.py

Lines 352 to 357 in 913b1cd

def test_duplicate_flags_raises_error(self):

parent_context = self.make_dbt_context("parent", ["--version-check"])

context = self.make_dbt_context("child", ["--version-check"], parent_context)

with pytest.raises(DbtUsageException):

Flags(context)

github-actions · 2023-09-28T11:36:23Z

The backport to 1.5.latest failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.5.latest 1.5.latest
# Navigate to the new working tree
cd .worktrees/backport-1.5.latest
# Create a new branch
git switch --create backport-8670-to-1.5.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f17c1f3fe730947f554b55e4e5471bb9cb45fd95
# Push it to GitHub
git push --set-upstream origin backport-8670-to-1.5.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.5.latest

Then, create a pull request where the base branch is 1.5.latest and the compare/head branch is backport-8670-to-1.5.latest.

(cherry picked from commit f17c1f3)

Fix #6497: Support global flags passed in after subcommands

fb305a1

aranke requested a review from a team as a code owner September 19, 2023 13:05

aranke requested a review from ChenyuLInx September 19, 2023 13:05

cla-bot bot added the cla:yes label Sep 19, 2023

aranke requested a review from MichelleArk September 19, 2023 13:07

aranke added 3 commits September 19, 2023 14:57

Don't put click.pass_context in global_flags

800c6b1

missed a spot

9f014bc

move a few flags out of global

efdedc3

aranke force-pushed the fix_6497 branch from 3901d22 to efdedc3 Compare September 20, 2023 09:51

jtcohen6 reviewed Sep 20, 2023

View reviewed changes

jtcohen6 added the user docs [docs.getdbt.com] Needs better documentation label Sep 20, 2023

Merge branch 'main' into fix_6497

913b1cd

aranke force-pushed the fix_6497 branch from 168f846 to 913b1cd Compare September 26, 2023 02:15

aranke mentioned this pull request Sep 26, 2023

[CT-3159] [spike] Globalize remaining Click flags #8715

Closed

4 tasks

aranke requested a review from jtcohen6 September 26, 2023 07:39

gshank approved these changes Sep 26, 2023

View reviewed changes

emmyoop approved these changes Sep 26, 2023

View reviewed changes

aranke merged commit f17c1f3 into main Sep 26, 2023
97 checks passed

aranke deleted the fix_6497 branch September 26, 2023 15:16

ChenyuLInx approved these changes Sep 26, 2023

View reviewed changes

jtcohen6 added backport 1.5.latest backport 1.6.latest labels Sep 28, 2023

github-actions bot mentioned this pull request Sep 28, 2023

[Backport 1.6.latest] Fix #6497: Support global flags passed in after subcommands #8732

Merged

github-actions bot pushed a commit that referenced this pull request Sep 28, 2023

Fix #6497: Support global flags passed in after subcommands (#8670)

c107422

(cherry picked from commit f17c1f3)

jtcohen6 pushed a commit that referenced this pull request Sep 28, 2023

Fix #6497: Support global flags passed in after subcommands (#8670)

1b70a35

jtcohen6 mentioned this pull request Sep 28, 2023

[Backport 1.5.latest] Fix #6497: Support global flags passed in after subcommands #8734

Merged

QMalcolm pushed a commit that referenced this pull request Oct 9, 2023

Fix #6497: Support global flags passed in after subcommands (#8670)

c5fecca

tlento mentioned this pull request Nov 6, 2023

Update typing-extensions version to >=4.4 #9012

Merged

5 tasks

tlento mentioned this pull request Feb 26, 2024

Update dbt-semantic-interfaces dependency to compatible range #9671

Merged

dbeatty10 mentioned this pull request Jun 14, 2024

[Bug] Environment variables take precedence over CLI flags that appear after the subcommand #10304

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6497: Support global flags passed in after subcommands #8670

Fix #6497: Support global flags passed in after subcommands #8670

aranke commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

jtcohen6 left a comment

jtcohen6 Sep 20, 2023

aranke commented Sep 20, 2023 •

edited

Loading

aranke commented Sep 20, 2023

jtcohen6 commented Sep 20, 2023

aranke commented Sep 20, 2023

aranke commented Sep 26, 2023 •

edited

Loading

jtcohen6 commented Sep 26, 2023

emmyoop left a comment

FishtownBuildBot commented Sep 26, 2023

ChenyuLInx left a comment

ChenyuLInx Sep 26, 2023

aranke Sep 26, 2023

github-actions bot commented Sep 28, 2023

	def test_duplicate_flags_raises_error(self):
	parent_context = self.make_dbt_context("parent", ["--version-check"])
	context = self.make_dbt_context("child", ["--version-check"], parent_context)

	with pytest.raises(DbtUsageException):
	Flags(context)

Fix #6497: Support global flags passed in after subcommands #8670

Fix #6497: Support global flags passed in after subcommands #8670

Conversation

aranke commented Sep 19, 2023 • edited Loading

Problem

Solution

Checklist

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 Sep 20, 2023

Choose a reason for hiding this comment

aranke commented Sep 20, 2023 • edited Loading

aranke commented Sep 20, 2023

jtcohen6 commented Sep 20, 2023

aranke commented Sep 20, 2023

aranke commented Sep 26, 2023 • edited Loading

jtcohen6 commented Sep 26, 2023

emmyoop left a comment

Choose a reason for hiding this comment

FishtownBuildBot commented Sep 26, 2023

ChenyuLInx left a comment

Choose a reason for hiding this comment

ChenyuLInx Sep 26, 2023

Choose a reason for hiding this comment

aranke Sep 26, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2023

aranke commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

aranke commented Sep 20, 2023 •

edited

Loading

aranke commented Sep 26, 2023 •

edited

Loading